chore: only copy current harness version to trial [DET-3681] #952
Conversation
There was a problem hiding this comment.
@stoksc YES! this is definitely been a very annoying in the past. This solution is good, only development corner case I see is if the version gets bumped and the harness isn't rebuilt, but that is much easier to debug/fix than what we had before.
master/pkg/tasks/copy.go
Outdated
| @@ -22,7 +24,8 @@ const ( | |||
|
|
|||
| func harnessArchive(harnessPath string, aug *model.AgentUserGroup) container.RunArchive { | |||
| var harnessFiles archive.Archive | |||
| wheelPaths, err := filepath.Glob(filepath.Join(harnessPath, "*.whl")) | |||
| validWhlNames := fmt.Sprintf("*%s*.whl", version.Version) | |||
| wheelPaths, err := filepath.Glob(filepath.Join(harnessPath, validWhlNames)) | |||
| if err != nil { | |||
| panic(errors.Wrapf(err, "error finding Python wheel files in path: %s", harnessPath)) | |||
There was a problem hiding this comment.
non-blocking: add in the the version we are looking for into the error message
There was a problem hiding this comment.
yeah sure, I thought about it but it seemed redundant since the det version should be obvious, but couldn't hurt.
| @@ -22,7 +24,8 @@ const ( | |||
|
|
|||
| func harnessArchive(harnessPath string, aug *model.AgentUserGroup) container.RunArchive { | |||
| var harnessFiles archive.Archive | |||
| wheelPaths, err := filepath.Glob(filepath.Join(harnessPath, "*.whl")) | |||
| validWhlNames := fmt.Sprintf("*%s*.whl", version.Version) | |||
| wheelPaths, err := filepath.Glob(filepath.Join(harnessPath, validWhlNames)) | |||
There was a problem hiding this comment.
Do we expect the wheelPaths to be predictable / something we should check?
There was a problem hiding this comment.
As far as I understand, it should always be $(ROOT)/wheels/determined*$(VERSION)*.whl where ROOT is root from master.yml and version is the version that is set by ldflags flag to go build when we build the master. Based on that, I guess this filepath.Glob could be even more specific.
There was a problem hiding this comment.
The names themselves are standardized by the PEP for whl's, https://www.python.org/dev/peps/pep-0427/#file-name-convention. (I didn't know this, was curious)
There was a problem hiding this comment.
Sorry, I mean len(wheelPaths) -- i.e., do we expect to find exactly 1 and should we check for that?
There was a problem hiding this comment.
Yeah, I think it should always be 3 (since determined, determined-cli and determined-common should be in $ROOT/wheels).
Description
Change the determined whl's we copy to the harness from
*.whlto*$(VERSION)*.whl.Test Plan
Commentary (optional)
This may be a naïve solution, but it seems just fine. The only thing I worry about is breaking some dev flow for the harness so that why I threw you on this, @aaron276h.